Skip to content

fix(issue): isTerminalNotification whitelist excludes all-complete / no-active-milestone exits (headless-events.js:62)#6173

Open
jeremymcs wants to merge 1 commit into
mainfrom
issue/5738-isterminalnotification-whitelist-exclude-1778892822
Open

fix(issue): isTerminalNotification whitelist excludes all-complete / no-active-milestone exits (headless-events.js:62)#6173
jeremymcs wants to merge 1 commit into
mainfrom
issue/5738-isterminalnotification-whitelist-exclude-1778892822

Conversation

@jeremymcs
Copy link
Copy Markdown
Collaborator

@jeremymcs jeremymcs commented May 16, 2026 •
edited by coderabbitai Bot
Loading

Summary

  • Expanded terminal notify prefix detection for complete/idle/no-active-milestone exits and verified with targeted headless tests (49/49 passing).

Verification

  • Completed in the repository worktree before push.

Related Issue

Repo

  • gsd-build/gsd-2

Branch

  • issue/5738-isterminalnotification-whitelist-exclude-1778892822

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced terminal state detection to recognize additional completion and idle scenarios
    • System now properly identifies auto-mode completion, idle states, and milestone status changes
    • Improved session termination handling for more operation states

Review Change Stack

@jeremymcs
fix(issue): isTerminalNotification whitelist excludes all-complete / …
f0c00a8 
…no-active-milestone exits (headless-events.js:62)
@jeremymcs jeremymcs mentioned this pull request May 16, 2026
Open
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a21bc46e-9f5d-44ab-b4cd-89c79a1dfd8c

📥 Commits

Reviewing files that changed from the base of the PR and between 8906b64 and f0c00a8.

📒 Files selected for processing (5)
  • packages/daemon/src/session-manager.ts
  • packages/mcp-server/src/session-manager.ts
  • src/headless-events.ts
  • src/tests/headless-detection.test.ts
  • src/tests/headless-v2-migration.test.ts
📝 Walkthrough

Walkthrough

This PR expands the TERMINAL_PREFIXES constant used by isTerminalNotification() across headless and package session managers. The whitelist now recognizes three additional terminal-state messages—auto-mode complete, no active milestone, and auto-mode idle—in addition to the existing auto-mode stopped and step-mode stopped prefixes. Tests are updated to verify detection of the new prefixes, and a new v1 migration test confirms auto-mode-complete notifications correctly mark session completion.

Changes

Terminal notification prefix expansion

Layer / File(s) Summary
Terminal prefix constant expansion
src/headless-events.ts, packages/daemon/src/session-manager.ts, packages/mcp-server/src/session-manager.ts		 The TERMINAL_PREFIXES constant is expanded from two prefixes (auto-mode stopped, step-mode stopped) to five, adding auto-mode complete, no active milestone, and auto-mode idle to recognize additional auto-mode exit states.
Terminal detection test coverage
src/tests/headless-detection.test.ts		 Test suite constant is updated to match the expanded prefix list, and new assertions are added to verify isTerminalNotification correctly matches the three newly recognized terminal prefixes.
V1 fallback completion test
src/tests/headless-v2-migration.test.ts		 Migration test constants are updated and a new test case confirms that an Auto-mode complete — ... terminal notification correctly triggers session completion (completed = true) and sets exit code to EXIT_SUCCESS in v1 mode.

🎯 2 (Simple) | ⏱️ ~8 minutes

🐰 Five prefixes now open the door,
Where three new exits were missed before,
Complete and idle and milestone's call,
Headless now catches them all! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: expanding the TERMINAL_PREFIXES whitelist to include missing auto-mode exit message prefixes (auto-mode complete, no active milestone, auto-mode idle).
Linked Issues check ✅ Passed All code changes implement the immediate compatibility fix proposed in issue #5738: broadening TERMINAL_PREFIXES across all necessary files and adding test coverage for the new terminal detection patterns.
Out of Scope Changes check ✅ Passed All changes are directly scoped to expanding TERMINAL_PREFIXES and adding corresponding tests; no unrelated modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue/5738-isterminalnotification-whitelist-exclude-1778892822

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 16, 2026

🟢 PR Risk Report — LOW
Files changed 5
Systems affected 1
Overall risk 🟢 LOW

Affected Systems

Risk System
🟢 low Headless Mode
File Breakdown
Risk File Systems
🟢 src/headless-events.ts Headless Mode
⚪ packages/daemon/src/session-manager.ts (unclassified)
⚪ packages/mcp-server/src/session-manager.ts (unclassified)
⚪ src/tests/headless-detection.test.ts (unclassified)
⚪ src/tests/headless-v2-migration.test.ts (unclassified)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

isTerminalNotification whitelist excludes all-complete / no-active-milestone exits (headless-events.js:62)

1 participant

@jeremymcs